Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "New Decimal <--> Floating conversion (#15905)" #16283

Closed
wants to merge 2 commits into from
Closed

Revert "New Decimal <--> Floating conversion (#15905)" #16283

wants to merge 2 commits into from

Conversation

ttnghia
Copy link
Contributor

@ttnghia ttnghia commented Jul 15, 2024 •
edited
Loading

After the PR #15905 merged, there was a performance regression reported:

We observed significant performance degradation to binary ops. We see 12-14x slowdowns in our AST benchmarks, plus 6-10x slowdowns in other binary ops.

This reverts the problematic PR (#15905), waiting for more investigation on the regression.

@ttnghia ttnghia added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. Performance Performance related issue non-breaking Non-breaking change labels Jul 15, 2024
@ttnghia ttnghia self-assigned this Jul 15, 2024
@ttnghia ttnghia requested review from a team as code owners July 15, 2024 19:51
@github-actions github-actions bot added Python Affects Python cuDF API. Java Affects Java cuDF API. labels Jul 15, 2024
@ttnghia ttnghia added the bug Something isn't working label Jul 15, 2024
vyasr
vyasr approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving based on offline discussions indicating that we think it's safe to go ahead without rebenchmarking.

@ttnghia
Copy link
Contributor Author

ttnghia commented Jul 15, 2024

/merge

@ttnghia ttnghia added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Jul 16, 2024
@abellina
Copy link
Contributor

@ttnghia just wondering what we are waiting for in this PR.

@ttnghia
Copy link
Contributor Author

ttnghia commented Jul 19, 2024

@ttnghia just wondering what we are waiting for in this PR.

There seems to be another fix merged in cudf so this may no longer be needed. I'm still waiting for confirmation before closing this.

@mattahrens
Copy link
Collaborator

This is the related perf fix PR: #16287. It's been merged, but still waiting for final confirmation that is fixed the issue.

@ttnghia ttnghia closed this Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt approved these changes

@vyasr vyasr vyasr approved these changes

@wence- wence- Awaiting requested review from wence- wence- is a code owner automatically assigned from rapidsai/cudf-python-codeowners

@isVoid isVoid Awaiting requested review from isVoid isVoid is a code owner automatically assigned from rapidsai/cudf-python-codeowners

@karthikeyann karthikeyann Awaiting requested review from karthikeyann karthikeyann is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@shrshi shrshi Awaiting requested review from shrshi shrshi is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees

@ttnghia ttnghia

Labels
3 - Ready for Review Ready for review by team 5 - DO NOT MERGE Hold off on merging; see PR for details bug Something isn't working Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Performance Performance related issue Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ttnghia @abellina @mattahrens @vyasr @davidwendt